-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BREAKING: Fix InvalidOperationException
when using PropertiesDictionary in a multithreaded application and remove [Serializable] for it.
#2415
Conversation
protected PropertiesDictionary(SerializationInfo info, StreamingContext context) | ||
: base(info, context) | ||
{ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if the serialization will be the same comparing previous vs current #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the file written are the same
I think you might need to change this as well. In reply to: 989748904 In reply to: 989748904 In reply to: 989748904 In reply to: 989748904 Refers to: src/Sarif/TypedPropertiesDictionary.cs:72 in bef3fbe. [](commit_id = bef3fbe, deletion_comment = False) |
Assert.Null(exception); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove one line #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -192,6 +227,10 @@ private PropertiesDictionary RoundTripThroughJson(PropertiesDictionary propertie | |||
new PerLanguageOption<StringSet>( | |||
FEATURE, nameof(StringSetProperty), defaultValue: () => { return STRINGSET_DEFAULT; }); | |||
|
|||
public static PerLanguageOption<StringSet> GetStringSetProperty(int i) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringSet is a actually class here, let me know
how about change to "GenerateStringSetProperty"
the Get word may be confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'StringSet' is correct. :) I can completely understand why 'GenerateStringGetProperty' seems right, though! :)
I think the cacheDescription part have problem before our changes. And we don't have a any test for that field. In reply to: 990377643 Refers to: src/Sarif/TypedPropertiesDictionary.cs:72 in bef3fbe. [](commit_id = bef3fbe, deletion_comment = False) |
@@ -73,33 +74,28 @@ public static class PropertiesDictionaryExtensionMethods | |||
StringSet stringSet = property as StringSet; | |||
if (stringSet != null) | |||
{ | |||
SaveSettingNameToDescriptionMap(writer, key, settingNameToDescriptionMap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found this new method is not needed, removed.
the bug was comment not written to file,
the method I add was to test that. PropertiesDictionary_TestCacheDescription
@@ -13,7 +13,7 @@ public interface IMarker { } | |||
|
|||
[Serializable] | |||
[JsonConverter(typeof(TypedPropertiesDictionaryConverter))] | |||
public class TypedPropertiesDictionary<T> : Dictionary<string, T>, IMarker where T : new() | |||
public class TypedPropertiesDictionary<T> : ConcurrentDictionary<string, T>, IMarker where T : new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the title of the PR and add a new entry to the ReleaseHistory.
The title should be something like: "Fix InvalidOperationException when using PropertiesDictionary in a multithreaded application".
With that, if any customer checks the release history, the customer would know what motivated us to change this from Dictionary to ConcurrentDictionary.
This should also be applied to the PR description. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done both, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also added the ReleaseHistory for the other 2 PRs.
@@ -37,11 +37,6 @@ public PropertiesDictionary() : this(null) | |||
{ | |||
} | |||
|
|||
protected PropertiesDictionary(SerializationInfo info, StreamingContext context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the .net core concurrentdictionary is not binary serializable, so I removed it,
question, do we even need it, it is not for json and xml serialization, it is only for BinaryFormatter serialization, in the solution I don't see code use BinaryFormatter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question and in future, definitely try to raise this before making this kind of change. Remember we are in an SDK, so it is possible that someone is using BinaryFormatter with this public type (even though we do not as you point out).
I think probably it is unlikely we're breaking someone out there. So this change is fine. However we need to very clearly mark this breaking change in behavior in the release notes.
Imagine someone using BInaryFormatter with this type, using an older SDK. Now they update to the new SDK and everything is broken. Ideally, your release notes would also mention the symptom these users would receive. Make sense? IOW, try to use BinaryFormatter again PropertiesDictionary (once you remove the [Serializable] attribute as well. See what exception is raised. Make sure the release notes reference this exception in case users experience it.
By doing this, we leave an easily followed trail of clues that tell developers what happened. That's the least we can do, given that we're introducing a breaking change. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the great inputs,
I have remove the [Serializable] attribute and ran it, the error is
SerializationException: Type PropertiesDictionary is not marked as serializable
I have added it both in the PR desc and also the release history md file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additional question, do you want me to create a work item to check and remove all [Serializable] in the solution,
( I have not dig if any of them is used somewhere yet, just to bring up the discussion )
https://docs.microsoft.com/en-us/dotnet/standard/serialization/binaryformatter-security-guide
https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2300
src/Sarif/ExtensionMethods.cs
Outdated
@@ -585,5 +586,15 @@ internal static void ConsumeElementOfDepth(this XmlReader xmlReader, int endElem | |||
xmlReader.Read(); | |||
} | |||
} | |||
|
|||
public static bool Add<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key, T value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just checked the source code of CurrentDictionary,
the IDictionary<TKey, TValue>.Add implementation
did call the same method as the TryAdd
so we can use it thread safely.
Also agree that we should not make the Add has a return value people will get confused.
updated the code.
src/Sarif/ExtensionMethods.cs
Outdated
@@ -585,5 +586,15 @@ internal static void ConsumeElementOfDepth(this XmlReader xmlReader, int endElem | |||
xmlReader.Read(); | |||
} | |||
} | |||
|
|||
public static bool Add<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key, T value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this extension method return a bool? IDictionary.Add returns void.
It's not a good idea to create a strange return value for 'Add', this will confuse experienced .NET programmers.
If you need to override 'Add', so that a concurrent dictionary behaves properly when it flows around to IDictionary-accepting things, this should return void. And in the case where Add would throw, you should consult the return value of TryAdd and throw on false.
#Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree and updated, see the other comment.
@@ -119,6 +115,19 @@ public static class PropertiesDictionaryExtensionMethods | |||
writer.WriteEndElement(); // Properties | |||
} | |||
|
|||
private static void SaveSettingNameToDescriptionMap(XmlWriter writer, string key, ConcurrentDictionary<string, string> settingNameToDescriptionMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we doing here? Seems like you found a different issue and are oportunistically fixing it?
[edit] oh, yes, I see the bug, because we 'continue', we didn't call this code.
Why didn't you just move the logical earlier in the method? And avoid injecting four calls to your new method? Is that possible? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there were no tests for this and when I add tests this issue show up and blocking my test pass so I should fix it together.
avoid injecting four calls to your new method ---- agree, let me take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed to only one copy. the method is not needed.
} | ||
|
||
[Fact] | ||
public void PropertiesDictionary_TestCacheDescription() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
InvalidOperationException
when using PropertiesDictionary in a multithreaded application
Let's remove this, if we're removing the serialization ctor. In reply to: 1004422690 In reply to: 1004422690 Refers to: src/Sarif/PropertiesDictionary.cs:18 in 8eb304d. [](commit_id = 8eb304d, deletion_comment = False) |
InvalidOperationException
when using PropertiesDictionary in a multithreaded applicationInvalidOperationException
when using PropertiesDictionary in a multithreaded application and remove [Serializable] for it.
fixed. In reply to: 1004422690 Refers to: src/Sarif/PropertiesDictionary.cs:18 in 8eb304d. [](commit_id = 8eb304d, deletion_comment = False) |
{ | ||
var properties = new PropertiesDictionary(); | ||
|
||
Exception exception = Record.Exception(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this test can repro the error before change the type.
@@ -585,5 +586,15 @@ internal static void ConsumeElementOfDepth(this XmlReader xmlReader, int endElem | |||
xmlReader.Read(); | |||
} | |||
} | |||
|
|||
public static void Add<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key, T value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these? Can't we just require our code to cast to IDictionary when appropriate?
What we've done here is effectively add to the public surface area of our SDK API. Do we want to do that?
My suggestion is to remove this and simply cast our internal concurrent dictionary instances to IDictionary. In order not to produce a new public API. #Closed
@@ -585,5 +586,15 @@ internal static void ConsumeElementOfDepth(this XmlReader xmlReader, int endElem | |||
xmlReader.Read(); | |||
} | |||
} | |||
|
|||
public static void Add<T>(this ConcurrentDictionary<string, T> concurrentDictionary, string key, T value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this more closely, I think what we need to do is to reintroduce IDictionary<string, T> on the TypedPropertiesDictionary class, it is currently not available because it is implemented explicitly in ConcurrentDictionary.
I chased this around a little bit and you may need to change the signatures of multiples types that extend the core typed dictionary. #Closed
…o that it can be used where IDictionary is expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description:
Bug:
InvalidOperationException
when using PropertiesDictionary in a multithreaded application.It is caused by use of Dictionary type instead of ConcurrentDictionary type.
Fixes:
Change TypedPropertiesDictionary to use ConcurrentDictionary.
also found a existing issue with method SavePropertiesToXmlStream, fix by the way.
BREAKING: removed [Serializable] for TypedPropertiesDictionary.
[Serializable] is not for json and xml serialization, it is only for BinaryFormatter serialization.
For SDK user that is currently using BinaryFormatter on TypedPropertiesDictionary object,
will result in error:
SerializationException
: Type PropertiesDictionary is not marked as serializableError Details:
InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
at System.Collections.Generic.Dictionary
2.FindEntry(TKey key) at System.Collections.Generic.Dictionary
2.TryGetValue(TKey key, TValue& value)at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.TryGetProperty[T](String key, T& value)
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.GetProperty[T](PerLanguageOption
1 setting, Boolean cacheDefault) at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.GetProperty[T](PerLanguageOption
1 setting)at Microsoft.CodeAnalysis.IL.Rules.EnableSpectreMitigations.AnalyzePortableExecutableAndPdb(BinaryAnalyzerContext context) in D:\a\r1\a\src\BinSkim.Rules\PERules\BA2024.EnableSpectreMitigations.cs:line 139
at Microsoft.CodeAnalysis.IL.Rules.WindowsBinaryAndPdbSkimmerBase.Analyze(BinaryAnalyzerContext context) in D:\a\r1\a\src\BinSkim.Rules\PERules\WindowsBinaryAndPdbSkimmerBase.cs:line 63
at Microsoft.CodeAnalysis.Sarif.Driver.MultithreadedAnalyzeCommandBase
2.AnalyzeTargetHelper(TContext context, IEnumerable
1 skimmers, ISet`1 disabledSkimmers)Test Case result before Fix:
Microsoft.CodeAnalysis.Sarif.PropertiesDictionaryTests.PropertiesDictionary_ConcurrentReadAndWrite
Duration: 389 ms
Message:
Assert.Null() Failure
Expected: (null)
Actual: System.AggregateException: One or more errors occurred. (Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.) (Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.)
---> System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
at System.Collections.Generic.Dictionary
2.FindEntry(TKey key) at System.Collections.Generic.Dictionary
2.TryGetValue(TKey key, TValue& value)at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.GetSettingsContainer(IOption setting, Boolean cacheDefault) in C:\GH\s-s\src\Sarif\PropertiesDictionary.cs:line 126
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.SetProperty(IOption setting, Object value, Boolean cacheDescription, Boolean persistToSettingsContainer) in C:\GH\s-s\src\Sarif\PropertiesDictionary.cs:line 78
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.SetProperty(IOption setting, Object value, Boolean cacheDescription) in C:\GH\s-s\src\Sarif\PropertiesDictionary.cs:line 70
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionary.SetProperty(IOption setting, Object value) in C:\GH\s-s\src\Sarif\PropertiesDictionary.cs:line 65
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionaryTests.<>c__DisplayClass16_1.<PropertiesDictionary_ConcurrentReadAndWrite>b__1() in C:\GH\s-s\src\Test.UnitTests.Sarif\PropertiesDictionaryTests.cs:line 146
at System.Threading.Tasks.Task.InnerInvoke()
at System.Threading.Tasks.Task.<>c.<.cctor>b__274_0(Object obj)
at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of inner exception stack trace ---
at System.Threading.Tasks.Task.WaitAllCore(Task[] tasks, Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.WaitAll(Task[] tasks)
at Microsoft.CodeAnalysis.Sarif.PropertiesDictionaryTests.<>c__DisplayClass16_0.<PropertiesDictionary_ConcurrentReadAndWrite>b__0() in C:\GH\s-s\src\Test.UnitTests.Sarif\PropertiesDictionaryTests.cs:line 161
at Xunit.Record.Exception(Action testCode) in C:\Dev\xunit\xunit\src\xunit.core\Record.cs:line 26